Skip to content

Comments

Remove an invalid assertion that triggers fuzzers#1492

Closed
kleisauke wants to merge 1 commit intostrukturag:masterfrom
kleisauke:remove-invalid-assert
Closed

Remove an invalid assertion that triggers fuzzers#1492
kleisauke wants to merge 1 commit intostrukturag:masterfrom
kleisauke:remove-invalid-assert

Conversation

@kleisauke
Copy link
Contributor

Found by libvips' fuzz tests.

$ heif-dec clusterfuzz-testcase-minimized-jpegsave_file_fuzzer-5001583624781824
heif-dec: /builddir/build/BUILD/libheif-1.19.7/libheif/bitstream.cc:169: BitstreamRange::BitstreamRange(std::shared_ptr<StreamReader>, size_t, size_t): Assertion `success' failed.
Aborted (core dumped)

See: libvips/libvips#4424.

@farindk
Copy link
Contributor

farindk commented Mar 19, 2025

Can you send me or attach the POC file?

@kleisauke
Copy link
Contributor Author

I've just send it to your privately as this is an issue in libvips.

FWIW, this assertion failure also occurs on Fedora because the RPM spec files didn't pass -DCMAKE_BUILD_TYPE=Release and libheif doesn't default to a release build when this parameter is omitted.

@farindk
Copy link
Contributor

farindk commented Mar 19, 2025

Thanks, I'll have a closer look at the POC what the root cause for this is.

@kleisauke
Copy link
Contributor Author

FWIW, this assertion failure also occurs on Fedora because the RPM spec files didn't pass -DCMAKE_BUILD_TYPE=Release and libheif doesn't default to a release build when this parameter is omitted.

@Conan-Kudo @rathann Sorry for the ping. I'm trying to determine the best approach for handling this from a packaging perspective. I noticed that the CMake macros pass -DCMAKE_{C,CXX}_FLAGS_RELEASE:STRING="-DNDEBUG":
https://src.fedoraproject.org/rpms/cmake/blob/rawhide/f/macros.cmake.in#_25

However, this would only work when libheif is built with -DCMAKE_BUILD_TYPE=Release. Should libheif default to a release build in this case, similar to libjpeg-turbo?:
https://github.com/libjpeg-turbo/libjpeg-turbo/blob/3.1.0/CMakeLists.txt#L88-L90

@farindk
Copy link
Contributor

farindk commented Mar 19, 2025

For libheif, there is also cmake --preset=release which provides a good basis set of parameters that you can customize during packaging.

@rathann
Copy link

rathann commented Mar 20, 2025

FWIW, this assertion failure also occurs on Fedora because the RPM spec files didn't pass -DCMAKE_BUILD_TYPE=Release and libheif doesn't default to a release build when this parameter is omitted.

@Conan-Kudo @rathann Sorry for the ping. I'm trying to determine the best approach for handling this from a packaging perspective. I noticed that the CMake macros pass -DCMAKE_{C,CXX}_FLAGS_RELEASE:STRING="-DNDEBUG": https://src.fedoraproject.org/rpms/cmake/blob/rawhide/f/macros.cmake.in#_25

However, this would only work when libheif is built with -DCMAKE_BUILD_TYPE=Release. Should libheif default to a release build in this case, similar to libjpeg-turbo?: https://github.com/libjpeg-turbo/libjpeg-turbo/blob/3.1.0/CMakeLists.txt#L88-L90

For Fedora packaging, the default build type should be -DCMAKE_BUILD_TYPE=RelWithDebInfo to avoid stripping debug info from built objects. The build process strips that and puts them into separate subpackages, which can be installed when necessary.

kleisauke added a commit to weserv/rpms that referenced this pull request Mar 21, 2025
kleisauke added a commit to weserv/rpms that referenced this pull request Mar 21, 2025
@kleisauke
Copy link
Contributor Author

For Fedora packaging, the default build type should be -DCMAKE_BUILD_TYPE=RelWithDebInfo to avoid stripping debug info from built objects. The build process strips that and puts them into separate subpackages, which can be installed when necessary.

I can confirm that adding -DCMAKE_BUILD_TYPE=RelWithDebInfo in the RPM spec file disables these assertions. Let me know if you'd like a PR for that at https://src.fedoraproject.org/rpms/libheif.

@farindk
Copy link
Contributor

farindk commented Mar 25, 2025

This issue has been resolved already in 014cb59 in the sequences branch, which will become v1.20.0. It will not throw the assertion anymore, but return a proper error message.

$ heif-dec clusterfuzz-testcase-minimized-jpegsave_file_fuzzer-5001583624781824 
Could not read HEIF/AVIF file: Invalid input: Unspecified: Insufficient input data

Thus, I propose that instead of removing the assertion that we wait for v1.20.0 where the invalid input is correctly detected.

@farindk farindk closed this in e2253ec Mar 25, 2025
@farindk
Copy link
Contributor

farindk commented Mar 25, 2025

I have backported the fix into the master branch.

@kleisauke
Copy link
Contributor Author

Great! I can confirm that commit e2253ec fixes this.

FWIW, issue https://issues.oss-fuzz.com/issues/404288018 is now public, which could help packagers verify if libheif was built with assertions enabled.

@kleisauke kleisauke deleted the remove-invalid-assert branch March 25, 2025 11:07
farindk added a commit that referenced this pull request Mar 29, 2025
farindk added a commit that referenced this pull request Mar 29, 2025
@farindk
Copy link
Contributor

farindk commented Mar 29, 2025

The backported fix e2253ec was wrong since it caused same images to not load correctly (#1498). I have replaced it with a corrected fix 9839c99.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants